Skip to content

Manager: fixes from PR #36 review#38

Merged
njaltran merged 16 commits into
mainfrom
feature/manager-adapt
Jul 5, 2026
Merged

Manager: fixes from PR #36 review#38
njaltran merged 16 commits into
mainfrom
feature/manager-adapt

Conversation

@njaltran

@njaltran njaltran commented Jul 4, 2026

Copy link
Copy Markdown
Owner

What

Follow-up to #36 (merged before these landed). Two commits: my pre-merge self-review simplification pass, plus fixes for the findings of a full review of #36.

Correctness

  • select_best can no longer restore a class-collapsed pass. Best-iteration snapshots now rank on report_score — accuracy penalized below any healthy score when a class sits under the recall floor — the same rule the gate uses. Previously a degenerate all-neutral pass (e.g. 0.65 with up-recall 0.0) that the collapse floor refused to ship could still be snapshotted as "best" and restored into the finals.
  • Finalize can't flip the verdict anymore. The explanations-in-hand invocation routes straight to the finalize node instead of re-running decide on the (possibly restored) report — which also removes the iteration-bump/history-dedup special cases and a wasted LLM rationale call.
  • Collapse-forced first retune takes the schedule. When the floor blocks an above-target pass, Sabina's proposal says proceed and carries no suggested_params; the gate previously "accepted" {} and re-ran Nadi's defaults, wasting an iteration and poisoning tried_params.
  • Perturb candidates carry both knobs. A single-key dict (best = iteration 1 defaults) made _same() falsely skip untried schedule entries. boost_factor is also clamped (_BOOST_MAX) — threshold was clamped, boost grew without bound.
  • clean_outputs() runs after Aurora succeeds, not at run start — a run that dies on its inputs (typo'd --data-dir) no longer wipes the previous run's deliverables first.

Cleanup

  • One shared load_dotenv in agents/env.py (was copy-pasted into finetune_finbert.py@nadi, touches your file again, same footprint as before).
  • sample_size passed into build_pipeline explicitly; the agents.manager._defaults private reach-in is gone.
  • HISTORY_DIR named once; _next_params index mapping simplified; mock formatting churn reverted; pipeline test fakes deduped.
  • docs/retune_loop.md no longer contradicts docs/finetune_runs.md on the fine-tune-vs-baseline verdict.

Surfaced, not fixed (Sabina's lane)

class_accuracy is 0.0 for a label with zero test rows — indistinguishable from a collapse, so a window missing a label can never clear the target (documented in retune_loop.md). Needs per-class support in evaluation_report.json.

Branches merged into this one

Since this PR was opened, two more branches landed on top of feature/manager-adapt:

  • Evaluator: score retunes on a val split; test set touched once #37 fix/eval-val-split — retune loop now evaluates on a val split instead of the test split (test is scored exactly once, at the end). Touches nadi_classifier.py, sabina_evaluator.py, jack_manager.py, finetune_finbert.py.
  • Share handoff file rules #39 feature/contracts-module — new agents/contracts.py centralizes the handoff-file column lists/validation that were previously duplicated across agents. Also includes my own --dataset-end CLI fix (restores a flag that had been accidentally reverted, so full-pipeline runs can exclude the COVID window from the test split again).

For my teammates (Freddi, Aurora, Sabina, Nadi)

The contracts centralization (#39) touches your agent files directly, not just mine. Please check the diff on your file specifically for behavior changes, don't assume it's purely mechanical:

  • Freddiagents/freddi_explanation.py
  • Auroraagents/aurora_processing.py
  • Sabinaagents/sabina_evaluator.py
  • Nadiagents/nadi_classifier.py

I also modified the notebooks below as part of that same change — I updated them to match, but I have not fully re-verified them end to end, so please check your own before trusting it:

  • docs/explanation_agent_walkthrough.ipynb (Freddi)
  • docs/nadi_classifier_guide.ipynb (Nadi)
  • docs/sabina_evaluator_guide.ipynb (Sabina)

Verification

  • 60 tests pass (uv run pytest tests/), including new regression tests: collapsed pass never outranks healthy, collapse-forced retune params, full-key perturbs, boost clamp, failed-start keeps previous outputs.

njaltran added 2 commits July 4, 2026 19:22
- Share one load_dotenv (agents/env.py) between main.py and the
  fine-tune script instead of two hand-rolled copies
- Thread sample_size into build_pipeline explicitly; drop the
  agents.manager._defaults reach-in
- Route the finalize pass straight to its node: decide loses both
  finalize special cases (iteration bump, history dedup) and the
  redundant LLM rationale call on a decision that cannot change
- Rank best-iteration snapshots with report_score (accuracy,
  collapse-penalized) — one definition shared with the gate's class
  floor, so select_best can never restore a degenerate pass
- Name classifier_history once (HISTORY_DIR), simplify _next_params
  index mapping, revert mock formatting churn, dedup pipeline test
  fakes and graph-build boilerplate
- Perturb candidates carry both knobs (a partial dict made _same()
  falsely skip untried schedule entries) and boost_factor is clamped
  at _BOOST_MAX
- A collapse-forced first retune falls back to the schedule instead of
  accepting the proceed-proposal's empty suggested_params
- clean_outputs runs after Aurora succeeds, so a run that dies on its
  inputs keeps the previous run's deliverables
- retune_loop.md: fix stale fine-tune-vs-baseline claim (contradicted
  finetune_runs.md); document the zero-support-class collapse limit
  (needs per-class support from Sabina to fix properly)
Copilot AI review requested due to automatic review settings July 4, 2026 17:35

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

aouirora and others added 9 commits July 4, 2026 19:35
The loop tuned thresholds against the same test rows the final report
was measured on — eight rounds of that overfits the test set. Now:

- Aurora: split gains 'val' (last 10% of training dates), same
  date-based no-leak logic; test window unchanged.
- Nadi's generated classifier predicts val + test rows, carrying each
  row's split value through (was: test only).
- Sabina: eval_split parameter ('val' | 'test') filters what she
  scores, hard error when the requested rows are absent; the report
  records eval_split. code_notes now flag class collapse (any class
  recall < 0.05), and the LLM prompt asks for per-class judgement and
  warns against chasing the aggregate number.
- Pipeline: loop evaluations run eval_split='val'; new evaluate_test
  node scores the selected best classifier on test exactly once before
  explain/finalize.
- Manager: explanation sample and finals filter to test rows (val rows
  exist only for the loop's own scoring).
- Contract, mock_data (2 rows now val; report regenerated), and docs
  updated together per the working agreement.

Cross-lane note: touches Aurora's and Nadi's modules at Jack's
direction — flagged for their review on the PR.

58 tests pass.
Evaluator: score retunes on a val split; test set touched once
e88f7e3 stripped --dataset-end from main.py/pipeline_graph.py, leaving
Aurora's dataset_end param reachable only by calling ProcessingAgent
directly. Full-pipeline runs since then silently test on whatever
window falls out of the split, including the COVID crash - producing
sub-random accuracy that looked like a model failure but was actually
an evaluation bug.
Force-added from gitignored outputs/ since these are the artifacts
of a completed run worth preserving (decision, reports, predictions,
explanations, finetune comparisons). Model weight dirs and the
classifier.py run-copy stay untracked.

@aouirora aouirora left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to me! Approving the 3-way split in processing agent (test / train /val)

@srudolph25 srudolph25 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@nadikyaw

nadikyaw commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

It seems ok to me

srudolph25 and others added 4 commits July 5, 2026 13:17
Test-row filtering lives in contracts._test_rows (used by
build_final_results + write_explanation_sample). The copy in
jack_manager had no callers after the contracts centralization —
a stale second definition a future edit could wrongly trust.
@njaltran njaltran merged commit d9516cb into main Jul 5, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants